-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: updated loaders #14594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
feat: updated loaders #14594
Conversation
|
|
@ascorbic this is very WIP but can you have a look at the code and let me know what you think about the approach? |
ascorbic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify the scope of what you are trying to fix here? As I understand it, there are a couple of issues that could be fixed by running the schema parse later:
- resolve references
- support refine etc with image helper (and generally improve the image handling)
- don't require loader authors to manually parse data
Is the goal to fix both of these, or soemthing else?
| new Traverse(data).forEach((_, val) => { | ||
| if (typeof val === 'string' && val.startsWith(IMAGE_IMPORT_PREFIX)) { | ||
| const src = val.replace(IMAGE_IMPORT_PREFIX, ''); | ||
| foundAssets.add(src); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're doing this two-pass approach, could we find a better way to handle images? This has always felt like a hack, but was needed because of the time when it had to run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, I chose the simplest approach possible for this POC by simply copying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this and I don't think the new approach would allow to get rid of this. Basically during the first pass we don't know which properties are images so we can't store them for processing before the 2nd pass IIUC
|
So the goal here is to support returning a schema from
So my solution to this problem was to have an intermediary store. It contains the initial parsed data and the loader can manipulate it by calling any method. Then once All the potential improvements you mentionned are basically unintended and out of scope of this PR (except |
|
It seems to me that if we're going to fix this, we should at the very least ensure this is designed this in such a way that the others can be fixed too, even if we don't fix them immediately |
|
Sure. Can you give me more info about the first 2 bullet points? Like how it works currently and why, and what improvement you'd like |
|
We currently can't check references because other collections may not have been loaded. If doing two passes, we can check that referenced entries exist. This should be a relatively easy win. With images, because of the way we do this placeholder thing and then process images later, we can't return useful metadata from the helper anymore, meaning refine() doesn't work. I'm not sure if this is possible to fix with this, because it may still be too early in the build, but it's worth looking at. |
|
@ascorbic I pushed a POC for early references validation, let me know what you think |
| new Traverse(data).forEach((ctx, value) => { | ||
| if (!isReference(value)) { | ||
| return; | ||
| } | ||
| // TODO: better data structure | ||
| const collectionStore = filteredRawLoaderResults.find( | ||
| (e) => e.name === value.collection, | ||
| )!.memoryStore; | ||
| const id = 'id' in value ? value.id : value.slug; | ||
| if (!collectionStore.has(id)) { | ||
| // TODO: AstroError | ||
| throw new Error( | ||
| `Invalid reference for ${name}.${ctx.keys!.map((key) => key.toString()).join('.')}: cannot find entry ${id}`, | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no way to do this inside the schema validation? Traversing always seems a bit of a hack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree. If reference was passed like image() we could have some context in there but AFAIK with the current implementation there's not much we can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point. I forgot it was handled like that. Now I'm trying to remember why it didn't need to traverse before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's because reference() is actually never checked? It would only throw at runtime when calling eg. getEntry(post.data.author)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pre-content layer it did check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah definitely, I didn't understand that's what you were talking about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so I looked and that's because createReference was passed the lookupMap
astro/packages/astro/src/content/runtime.ts
Line 607 in 32b14f2
| const entry = store.get(collection, lookup); |
|
I think the POC works fine so now I'll work on cleaning everything up, tests, docs etc |
Todo
Changes
pnpm changeset.Testing
Docs